sRGB awareness for Color #616
Conversation
karroffel
added
C-Feature
|
Brilliant. At first glance this all looks good. However I think I would value "const-ness" over having a trait for conversions. Can we just expose the conversion logic as const functions and use those? |
|
I've tried that, but also the conversion logic relies on |
|
Hmm then I guess we'll need to wait / not be const anymore. Turns out float-ops in const functions is a much more complicated topic than I thought 😄 |
Co-authored-by: Gray Olson <[email protected]>
Co-Authored-By: Gray Olson <[email protected]>
|
Something that will almost certainly be an issue with this new api: let mut color = Color::rgb(0.6, 0.0, 0.0);
assert!(color.r != 0.6);
color.r = 0.6;
assert!(color.r == 0.6);This will be confusing for people. I think we need to make the color fields private and expose them via srgb getters/setters. |
There was a problem hiding this comment.
Great work! This looks good to me for now, once we wrap up the comment below. I think there are two followups to this pr we should do:
- Adjust example colors to use srgb. I'll do that!
- Try resolving the "to/from conversion precision" issue, as illustrated by
test_color_components_roundtrip. We could, for example, store a colorspace enum internally (ex:struct Color { colorspace: ColorSpace::srgb(1.0, 0.0, 0.0, 0.0) }). The main drawback is that the inner machinery is more complicated. We would still want the r() g() b() methods to return srgb colorspace and it should byte-convert to linear. We could also consider having a Color type (which is stored as srgb) and ColorLinear (which is stored as linear). However this can't happen yet because its currently assumed Color is directlyByteableto linear bytes. I plan on rethinking the Uniform/RenderResources derives, which might alleviate this restriction.
|
Haha one more request. As this is a change that will immediately break people on master, can we add a changelog entry in this pr? |
|
Like this? |
|
@julhe I suggest something like this: |
Co-Authored-By: Nathan Stocks <[email protected]>
Co-Authored-By: Nathan Stocks <[email protected]>
|
Alrighty lets merge this :) |
#585 raised awareness for a sRGB aware
Colorstruct. Basically, colors that where imported from Photoshop, GIMP or any other application will not match up with bevy, as bevy interpretate the color as linear, while it is in sRGB.Summary
Color::rgbandColor::rgbaare now sRGB aware per default, so the color will be transformed into linear space.Color::rgb_linearandColor::rgba_linearallows for linear colors.SrgbColorSpaceallows for conversion onf32level. I explicitly didn't implemented the trait forColoras I didn't want even the possiblity to treatColoras a sRGB color. It is suppose to be linear, always. 😈Follow-up Issues
Color::rgbbe actuallyColor::rgb_srgb? (Thats confusing in my opinion, but feedback is welcome)Color::rgbandColor::rgbacan't be used forconstsince they relly on traits, see Meta tracking issue forconst fnrust-lang/rust#57563. (Color::rgb_linearandColor::rgba_linearhowever can)Color::rgbandColor::rgbaare darker now.